-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Upgrades] v0.0.12 upgrade #1043
base: main
Are you sure you want to change the base?
Conversation
app/upgrades/v0.0.12.go
Outdated
// - Before: v0.0.11 | ||
// - After: v0.0.12 | ||
|
||
// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: https://github.com/pokt-network/poktroll/compare/v0.0.11...feat/proof-endblocker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: v0.0.11...feat/proof-endblocker
app/upgrades/v0.0.12.go
Outdated
// - After: v0.0.12 | ||
|
||
// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: https://github.com/pokt-network/poktroll/compare/v0.0.11...feat/proof-endblocker | ||
// TODO_IN_THIS_PR: Wait for https://github.com/pokt-network/poktroll/pull/1042 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | ||
StakingFee: &cosmosTypes.Coin{ | ||
Denom: "upokt", | ||
// TODO_IN_THIS_PR: 100upokt a good value? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: 100upokt a good value?
app/upgrades/v0.0.12.go
Outdated
} | ||
logger.Info("Successfully updated supplier params", "new_params", supplierParams) | ||
|
||
// TODO_IN_THIS_PR: RevSharePercent / DefaultRevSharePercent has been changed from float32 to uint64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: RevSharePercent / DefaultRevSharePercent has been changed from float32 to uint64.
app/upgrades/v0.0.12.go
Outdated
// and maintain two versions of protobuf files IF we need to loop through existing suppliers and update their onchain | ||
// data. | ||
|
||
// TODO_IN_THIS_PR: Add service.params.target_num_relays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: Add service.params.target_num_relays.
app/upgrades/v0.0.12.go
Outdated
// data. | ||
|
||
// TODO_IN_THIS_PR: Add service.params.target_num_relays. | ||
// TODO_IN_THIS_PR: Add tokenomics.params.global_inflation_per_claim. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: Add tokenomics.params.global_inflation_per_claim.
app/upgrades/v0.0.12.go
Outdated
// protobuf field. As a result, we expect existing on-chain data to switch to default value. | ||
// Investigate the impact of this change on existing on-chain data. | ||
// | ||
// TODO_IN_THIS_PR: decide if we need a proper module migration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_PR: decide if we need a proper module migration.
5a3decc
to
f187c35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I feel we are starting to improve our migration plans 👏
}, | ||
StakingFee: &cosmosTypes.Coin{ | ||
Denom: "upokt", | ||
// TODO_IN_THIS_PR: 100upokt a good value? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 100upokt
or 1000upokt
is a good value for now. We have the transaction fees that also protect from sibyl.
// Force all services to have a 100% revshare to the supplier. | ||
// Not something we would do on a real mainnet, but it's a quick way to resolve the issue. | ||
// Currently, we don't break any existing suppliers (as all of them have a 100% revshare to the supplier). | ||
service.RevShare = []*sharedtypes.ServiceRevenueShare{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would be able to loop over the old revshare map by unmashalling to the old proto type then create the new Supplier
proto with the new values.
But since we don't have proto versioning and we marked the old (float32) value as reserved, this is the next best option offered to us.
Summary
Adding an upgrade handler for
v0.0.12
Type of change
Select one or more from the following:
consensus-breaking
label if so. See [Infra] Automatically add theconsensus-breaking
label #791 for detailsSanity Checklist
assignees
,reviewers
,labels
,project
,iteration
andmilestone
make docusaurus_start
make go_develop_and_test
andmake test_e2e
devnet-test-e2e
label to run E2E tests in CI